[SAML Configuration] Display domains to members and admins in Workspaces tab#72713
Conversation
mhawryluk
changed the title
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: View the translation diffdiff --git a/src/languages/de.ts b/src/languages/de.ts
index 01f5088e..406b2028 100644
--- a/src/languages/de.ts
+++ b/src/languages/de.ts
@@ -674,6 +674,7 @@ const translations = {
pinned: 'Angeheftet',
read: 'Gelesen',
copyToClipboard: 'In die Zwischenablage kopieren',
+ domains: 'Domänen',
},
supportalNoAccess: {
title: 'Nicht so schnell',
@@ -2617,7 +2618,7 @@ ${amount} für ${merchant} - ${date}`,
descriptionTwo: 'Kategorisieren und taggen Sie Ausgaben',
descriptionThree: 'Berichte erstellen und teilen',
},
- price: 'Testen Sie es 30 Tage lang kostenlos, dann upgraden Sie für nur <strong>$5/Monat</strong>.',
+ price: 'Teste es 30 Tage kostenlos, anschließend für nur <strong>$5/Monat</strong> upgraden.',
createWorkspace: 'Arbeitsbereich erstellen',
},
confirmWorkspace: {
diff --git a/src/languages/fr.ts b/src/languages/fr.ts
index 0715f61e..1e90aad6 100644
--- a/src/languages/fr.ts
+++ b/src/languages/fr.ts
@@ -672,6 +672,7 @@ const translations = {
pinned: 'Épinglé',
read: 'Lu',
copyToClipboard: 'Copier dans le presse-papiers',
+ domains: 'Domaines',
},
supportalNoAccess: {
title: 'Pas si vite',
@@ -2613,7 +2614,7 @@ ${amount} pour ${merchant} - ${date}`,
descriptionTwo: 'Catégoriser et étiqueter les dépenses',
descriptionThree: 'Créer et partager des rapports',
},
- price: "Essayez-le gratuitement pendant 30 jours, puis passez à l'abonnement pour seulement <strong>5 $/mois</strong>.",
+ price: 'Essayez gratuitement pendant 30 jours, puis passez à la version payante pour seulement <strong>$5/mois</strong>.',
createWorkspace: 'Créer un espace de travail',
},
confirmWorkspace: {
diff --git a/src/languages/it.ts b/src/languages/it.ts
index d7a29b41..95df534b 100644
--- a/src/languages/it.ts
+++ b/src/languages/it.ts
@@ -672,6 +672,7 @@ const translations = {
pinned: 'Fissato',
read: 'Letto',
copyToClipboard: 'Copia negli appunti',
+ domains: 'Domini',
},
supportalNoAccess: {
title: 'Non così in fretta',
@@ -2628,8 +2629,8 @@ ${amount} per ${merchant} - ${date}`,
descriptionTwo: 'Categorizza e tagga le spese',
descriptionThree: 'Crea e condividi rapporti',
},
- price: 'Provalo gratis per 30 giorni, poi passa al piano superiore per soli <strong>$5/mese</strong>.',
- createWorkspace: 'Crea workspace',
+ price: "Provalo gratis per 30 giorni, poi esegui l'upgrade a soli <strong>$5/mese</strong>.",
+ createWorkspace: 'Crea spazio di lavoro',
},
confirmWorkspace: {
title: 'Conferma workspace',
diff --git a/src/languages/ja.ts b/src/languages/ja.ts
index bfa3e411..ea5e77e3 100644
--- a/src/languages/ja.ts
+++ b/src/languages/ja.ts
@@ -672,6 +672,7 @@ const translations = {
pinned: '固定済み',
read: '既読',
copyToClipboard: 'クリップボードにコピー',
+ domains: 'ドメイン',
},
supportalNoAccess: {
title: 'ちょっと待ってください',
@@ -2617,7 +2618,7 @@ ${date} - ${merchant}に${amount}`,
descriptionTwo: '経費を分類してタグ付けする',
descriptionThree: 'レポートを作成して共有する',
},
- price: '30日間無料でお試しいただけます。その後、<strong>$5/月</strong>でアップグレードしてください。',
+ price: '30日間無料でお試しください。以降は<strong>$5/月</strong>でアップグレードできます。',
createWorkspace: 'ワークスペースを作成',
},
confirmWorkspace: {
diff --git a/src/languages/nl.ts b/src/languages/nl.ts
index 5221cffe..bd8594b0 100644
--- a/src/languages/nl.ts
+++ b/src/languages/nl.ts
@@ -671,6 +671,7 @@ const translations = {
pinned: 'Vastgezet',
read: 'Gelezen',
copyToClipboard: 'Kopiëren naar klembord',
+ domains: 'Domeinen',
},
supportalNoAccess: {
title: 'Niet zo snel',
diff --git a/src/languages/pl.ts b/src/languages/pl.ts
index fe39b9ee..3c9a8b71 100644
--- a/src/languages/pl.ts
+++ b/src/languages/pl.ts
@@ -672,6 +672,7 @@ const translations = {
pinned: 'Przypięte',
read: 'Przeczytane',
copyToClipboard: 'Skopiuj do schowka',
+ domains: 'Domeny',
},
supportalNoAccess: {
title: 'Nie tak szybko',
@@ -2623,7 +2624,7 @@ ${amount} dla ${merchant} - ${date}`,
descriptionTwo: 'Kategoryzuj i taguj wydatki',
descriptionThree: 'Twórz i udostępniaj raporty',
},
- price: 'Wypróbuj za darmo przez 30 dni, a następnie przejdź na wyższy plan za jedyne <strong>5 USD/miesiąc</strong>.',
+ price: 'Wypróbuj za darmo przez 30 dni, a następnie uaktualnij za jedyne <strong>$5/mies.</strong>',
createWorkspace: 'Utwórz przestrzeń roboczą',
},
confirmWorkspace: {
diff --git a/src/languages/pt-BR.ts b/src/languages/pt-BR.ts
index e8b87062..e1fec78e 100644
--- a/src/languages/pt-BR.ts
+++ b/src/languages/pt-BR.ts
@@ -674,6 +674,7 @@ const translations = {
pinned: 'Fixado',
read: 'Lido',
copyToClipboard: 'Copiar para a área de transferência',
+ domains: 'Domínios',
},
supportalNoAccess: {
title: 'Não tão rápido',
@@ -2621,7 +2622,7 @@ ${amount} para ${merchant} - ${date}`,
descriptionTwo: 'Categorizar e etiquetar despesas',
descriptionThree: 'Criar e compartilhar relatórios',
},
- price: 'Experimente gratuitamente por 30 dias, depois faça o upgrade por apenas <strong>US$5/mês</strong>.',
+ price: 'Experimente grátis por 30 dias e depois faça o upgrade por apenas <strong>$5/mês</strong>.',
createWorkspace: 'Criar espaço de trabalho',
},
confirmWorkspace: {
diff --git a/src/languages/zh-hans.ts b/src/languages/zh-hans.ts
index 653cf6ca..c754d955 100644
--- a/src/languages/zh-hans.ts
+++ b/src/languages/zh-hans.ts
@@ -673,6 +673,7 @@ const translations = {
pinned: '已固定',
read: '已读',
copyToClipboard: '复制到剪贴板',
+ domains: '域名',
},
supportalNoAccess: {
title: '慢一点',
@@ -2594,7 +2595,7 @@ ${merchant}的${amount} - ${date}`,
descriptionTwo: '分类和标记费用',
descriptionThree: '创建和分享报告',
},
- price: '免费试用30天，然后只需<strong>$5/月</strong>升级。',
+ price: '免费试用30天，之后仅需<strong>$5/月</strong>即可升级。',
createWorkspace: '创建工作区',
},
confirmWorkspace: {
Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
| type GetWorkspaceMenuItem = {item: WorkspaceItem; index: number}; | ||
|
|
||
| type DomainItem = ListItem & {title: string; action: () => void; disabled: boolean}; | ||
|
|
||
| // eslint-disable-next-line react/no-unused-prop-types | ||
| type GetDomainMenuItem = {item: DomainItem; index: number}; |
There was a problem hiding this comment.
Is this eslint rule necessary? I see that we use both item and index later in the codebase. Besides, do we need to pass index to GetWorkspaceMenuItem if we disable the rule that checks if all props get used?
There was a problem hiding this comment.
it shouldn't be necessary, but there's an error without it. no idea why :/ and I think disabling this rule won't make it so that we pass a function with a wrong signature to the component, if that's what you mean. I think this rule should just check if the component uses all of its props in its implementation
| <PressableWithoutFeedback | ||
| role={CONST.ROLE.BUTTON} | ||
| accessibilityLabel="row" | ||
| style={[styles.mh5]} |
There was a problem hiding this comment.
If it's just 1 style I would avoid passing it in an array. Direct object will suffice, otherwise we add overhead of flattening the array. Not a blocker, just a minor optimisation
| text={translate('workspace.new.newWorkspace')} | ||
| onPress={() => interceptAnonymousUser(() => Navigation.navigate(ROUTES.WORKSPACE_CONFIRMATION.getRoute(ROUTES.WORKSPACES_LIST.route)))} | ||
| icon={Expensicons.Plus} | ||
| style={[shouldUseNarrowLayout && [styles.flexGrow1, styles.mb3]]} |
There was a problem hiding this comment.
If this can be flattened that would be awesome 😄
| ref={listRef} | ||
| sections={sections} | ||
| sectionTitleStyles={[styles.ph5, styles.pb5, styles.mt0, styles.mb0, styles.pt3]} | ||
| onSelectRow={() => {}} |
There was a problem hiding this comment.
This is not the first time in the codebase when we pass an empty function to onSelectRow. After this gets merged it's gonna be 3/8 usages. I think that we can set onSelectRow as an optional prop for SelectionList
| renderItem: getDomainMenuItem, | ||
| }, | ||
| ] as Array<SectionListDataType<WorkspaceItem | DomainItem>>, | ||
| [domains, filteredWorkspaces, getDomainMenuItem, getListHeaderComponent, getWorkspaceMenuItem, translate], |
There was a problem hiding this comment.
getListHeaderComponent is not wrapped in useCallback which means that the memoization won't work, because it's gonna be recreated every render
There was a problem hiding this comment.
eslint stopped working for me and I didn't realize 😶
Codecov Report❌ Patch coverage is
... and 1683 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| pinned: 'Przypięte', | ||
| read: 'Przeczytane', | ||
| copyToClipboard: 'Skopiuj do schowka', | ||
| domains: 'Domeny', |
melvin-bot
Bot
requested review from
ZhenjaHorbach
and removed request for
a team
|
@ZhenjaHorbach Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| const domains = useMemo( | ||
| () => | ||
| Object.values(allDomains ?? {}) | ||
| .filter((domain) => domain !== undefined) | ||
| .map((domain) => ({ | ||
| title: Str.extractEmailDomain(domain.email), | ||
| action: navigateToDomain, | ||
| disabled: !adminAccess?.[`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_ADMIN_ACCESS}${domain.accountID}`], | ||
| pendingAction: domain.pendingAction, | ||
| })) satisfies DomainItem[], | ||
| [navigateToDomain, allDomains, adminAccess], | ||
| ); |
There was a problem hiding this comment.
Let's optimize this code a bit
const domains = useMemo(() => {
if (!allDomains) return [];
return Object.values(allDomains).reduce<DomainItem[]>((acc, domain) => {
if (!domain) return acc;
acc.push({
title: Str.extractEmailDomain(domain.email),
action: navigateToDomain,
disabled: !adminAccess?.[`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_ADMIN_ACCESS}${domain.accountID}`],
pendingAction: domain.pendingAction,
});
return acc;
}, []);
}, [navigateToDomain, allDomains, adminAccess]);
| style={[shouldUseNarrowLayout && [styles.flexGrow1, styles.mb3]]} | ||
| /> | ||
| const getHeaderButton = () => | ||
| workspaces.length > 0 ? ( |
There was a problem hiding this comment.
!!workspaces.length ? (
| [ | ||
| // workspaces empty state | ||
| { | ||
| data: workspaces.length === 0 ? [{}] : [], |
There was a problem hiding this comment.
Could you please clarify this moment?
There was a problem hiding this comment.
when there are no workspaces, we create here a new section for the selection list, that shows the "you have no workspaces" empty state component. the section then has one element and it's an empty object, cause since there is only at-most one element to render, renderItem function doesn't need any input parameters
|
@mhawryluk Can you please take a look at the failing jest/ eslint? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2025-10-23.09.32.43.movAndroid: mWeb Chrome2025-10-23.09.39.14.moviOS: HybridApp2025-10-23.09.27.26.moviOS: mWeb Safari2025-10-23.09.24.31.movMacOS: Chrome / Safari2025-10-23.09.21.01.movMacOS: Desktop2025-10-23.09.21.01.mov |
Just purely for the reason that you can take the other actions like ask to be admin there at that point which you cannot do in NewDot as part of this release But I dont feel strongly about that |
We could just swap to FlashList, sorry, we dont have to use SelectionList here. The main thing is to do it in separate PR risk of regressions. However, without the change, the users would see that janky scrolling you and @ZhenjaHorbach reported above, right? If yes then I think we should swap to FlashList and then merge this to fix that issue |
|
I don't experience the janky animation bug with FlatList, I believe this is an issue specifically with React Native's SectionList |
When I sign in with an account that is not an admin of any domain, the Domains tab in OldDot settings isn't visible nor accessible, so I don't think we should navigate there |
I think its only if you are on domain that is claimed/ or verified |
|
Ok so are we good here from UX perspective @ZhenjaHorbach @mhawryluk? i am slightly confused if the bug you found is repro on the current shape of the branch |
|
I believe that the current state of this branch (we're back on FlatList) should work and that the bug is now fixed. I tested it, I even tried setting lower values for windowSize to try and reproduce it now, but it seems to work (so the bug was only happening when using the old SelectionList which uses SectionList under the hood; both FlatList and FlashList work fine). but I leave it now for @ZhenjaHorbach for testing |
|
all the review and design review comments (including the decision to not show domains when searching for a workspace) should also now be resolved btw ^^ |
NikkiWines
left a comment
NikkiWines
left a comment
There was a problem hiding this comment.
Looks good! Two very small comments
| return []; | ||
| } | ||
|
|
||
| return Object.values(allDomains).reduce<DomainItem[]>((acc, domain) => { |
There was a problem hiding this comment.
This variable name isn't very clear, maybe something like
| return Object.values(allDomains).reduce<DomainItem[]>((acc, domain) => { | |
| return Object.values(allDomains).reduce<DomainItem[]>((domainItems, domain) => { |
| const data = useMemo(() => { | ||
| const shouldShowDomainsSection = !inputValue.trim().length && domains.length; | ||
|
|
||
| return [ | ||
| !workspaces.length ? [{listItemType: 'workspaces-empty-state' as const}] : [], | ||
| filteredWorkspaces, | ||
| shouldShowDomainsSection ? [{listItemType: 'domains-header' as const}, ...domains] : [], | ||
| ].flat(); | ||
| }, [domains, filteredWorkspaces, workspaces.length, inputValue]); |
There was a problem hiding this comment.
NAB: Might be good to add a comment here about what data contains (workspaces, domain header, and domains) for clarity
|
@NikkiWines implemented the suggestions, thanks |
|
Sorry to bring this up again I thought we wanted the same hover color as when hovering over the arrow in a workspace item 2025-10-23.09.13.20.mov |
|
And it seems that after the latest commits, domains are not displayed after searching @trjExpensify During search, should we always show domains? 2025-10-23.09.35.11.mov |
|
But overall, the changes look good! And if we want, we can merge this PR |
|
I also noticed that the icon colors don't match the workspaces row, but I guess they do match most of the other menu items in the app. About the domains not being displayed when searching for a workspace, there was a discussion about it on the issue #72512 (comment) We decided to not display domains when searching for workspaces for now; in the future we will add the ability to search for domains. |
Nice then |
I agree I think the icon should have same hover colour @mhawryluk did the design team say otherwise before? if no, lets fix it please |
|
LGTM |
. Thank you for your hard work!
HelpDot Documentation ReviewOverall AssessmentThis PR does not contain any HelpDot documentation changes. The PR is focused on implementing a UI feature to display domains in the Workspaces tab alongside workspace information. All changes are code-related, including new React components, language translations, and UI modifications. Documentation Status❗ No documentation files (.md) were modified in this PR Analysis Summary
RecommendationsSince this PR introduces new user-facing functionality (displaying domains in the Workspaces tab with clickable admin access), consider whether:
Next StepsIf this feature requires user documentation:
Note: This assessment found no documentation changes to review. The PR contains only code implementation. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.37-1 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.2.37-8 🚀
|
8 participants
Explanation of Change
Replaces the list of workspaces in the Workspaces tab with a SelectionList with sections, one for workspaces and one for domains. Clicking on a domain row links to old dot.
Fixed Issues
$ #72512
PROPOSAL: N/A
Tests
Offline tests
Go offline. Perform the same steps as online tests, with the same outcome
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Nagranie.z.ekranu.2025-10-21.o.16.48.01.mov
Android: mWeb Chrome
Nagranie.z.ekranu.2025-10-21.o.16.50.46.mov
iOS: Native
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-10-21.at.16.36.32.mp4
iOS: mWeb Safari
(non-admin, non-clickable domain row)
Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-10-21.at.16.45.43.mp4
MacOS: Chrome / Safari
Nagranie.z.ekranu.2025-10-22.o.12.00.08.mov
MacOS: Desktop
Nagranie.z.ekranu.2025-10-22.o.12.08.15.mov